Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: re-add zeebe-user-task rule as a warning #183

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Nov 29, 2024

Related to camunda/camunda-modeler#4690

Added a new reporting category. Base message uses the terminology "is recommended to" as opposed to "must", since this is just the base message I think this is a good generic error here.

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 29, 2024
@philippfromme
Copy link
Collaborator

I will have a look today.

@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

If the feature is going to be removed at some point of time, let's mark it as deprecated. This is what we do with secrets. We could also add the support removal version if possible, though for the specific feature from this PR it has been fluent, so I'd rather not recommend it here.

Regarding recommendations in general, I don't even think there's space for such category in this repository. This would be an excellent case for a best-practices plugin (bpmnlint-plugin-camunda?). Note this project's mission:

A bpmnlint plug-in that checks whether a given BPMN process can be executed with Camunda.

I am collecting potential configurable rules in https://github.com/camunda/product-hub/issues/2013, so maybe there's something to contribute there for the user tasks too.

index.js Outdated
@@ -162,6 +163,7 @@ const rules = {
'no-zeebe-properties': './rules/camunda-cloud/no-zeebe-properties',
'no-zeebe-user-task': './rules/camunda-cloud/no-zeebe-user-task',
'priority-definition': './rules/camunda-cloud/priority-definition',
'recommend-zeebe-user-task': './rules/camunda-cloud/recommend-zeebe-user-task',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to something like no-job-worker-user-task.

@barmac
Copy link
Contributor

barmac commented Dec 2, 2024

Code-wise, the contribution looks solid. So we only need to adjust the message.

Copy link
Contributor

@barmac barmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the comment above.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 2, 2024
@philippfromme
Copy link
Collaborator

We already had a rule but removed it: #179 Feels like we're going in circles. ♻️

@barmac
Copy link
Contributor

barmac commented Dec 3, 2024

Sorry for confusion. Yes, we removed the rule because the job worker implementation is kept in the upcoming release. My suggestion is to notify deprecation, and we can bring the rule you linked back when/if the job worker is actually removed.

@barmac
Copy link
Contributor

barmac commented Dec 3, 2024

@barmac
Copy link
Contributor

barmac commented Dec 3, 2024

Hmm the comment that I linked makes me doubt what message we want to send in compat. Let's pause and clarify further in the epic linked.

@Skaiir
Copy link
Contributor Author

Skaiir commented Dec 11, 2024

We had some discussions with @barmac sync to try and settle on the specific language / error type involved. We decided, without alterations, to use the same exact message and error type for "no extension is deprecated" as "must have extension" case. We came to this decision because the pattern was already in use with another warning.
The actual differentiation in language and the term "deprecated" will be defined at the camunda/linting level instead and we keep things simple here.

What this basically amounts to is that we are re-adding the zeebe-user-task rule, but as a warning. So we are indeed going around in circles a little bit. But now we're doing it with a plan 😉.

@Skaiir Skaiir force-pushed the 4690-deprecate-job-worker-user-task branch from 73d68ba to 8048cdf Compare December 11, 2024 04:47
@Skaiir Skaiir changed the title feat: add recommend-zeebe-user-task rule feat: re-add zeebe-user-task rule as a warning Dec 11, 2024
@Skaiir Skaiir requested a review from barmac December 11, 2024 05:02
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 11, 2024
@Skaiir Skaiir force-pushed the 4690-deprecate-job-worker-user-task branch from 8048cdf to ef4c964 Compare December 11, 2024 08:20
@barmac
Copy link
Contributor

barmac commented Dec 11, 2024

Thanks!

@barmac barmac merged commit cab2c29 into main Dec 11, 2024
3 checks passed
@barmac barmac deleted the 4690-deprecate-job-worker-user-task branch December 11, 2024 09:56
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 11, 2024
@barmac
Copy link
Contributor

barmac commented Dec 11, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants